[DO NOT MERGE] Temporary Multiasset parametrization PR #1803
Conversation
… added vcoin, vinject and vsize to the type class Val 3. defined these for Value
…rdano-ledger-specs into polina-nonbreaking-ma
…Combinators.hs Examples.hs Init.hs
Examples.hs Examples/PoolLifetime.hs Examples/Updates.hs Fees.hs MultiSigExamples.hs
polinavino
requested review from
JaredCorduan,
nc6 and
uroboros
as code owners
| vcoin (Value c1 _) = c1 | ||
| vinject c1 = Value c1 vzero | ||
| vsize (Value _ v) = foldr accum uint v where -- add uint for the Coin portion in this size calculation | ||
| accum u ans = foldr accumIns (ans + addrHashLen) u where -- add addrHashLen for each Policy ID |
There was a problem hiding this comment.
Nit: it always takes me ages to remember the arguments to fold functions. I highly recommend type signatures for all local functions, tbh.
shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/Value.hs
Show resolved
Hide resolved
| visZero (Value c1 v1) = (visZero c1) && (visZero v1) | ||
| vcoin (Value c1 _) = c1 | ||
| vinject c1 = Value c1 vzero | ||
| vsize (Value _ v) = foldr accum uint v where -- add uint for the Coin portion in this size calculation |
There was a problem hiding this comment.
I'd find this whole thing much easier to read if it was written with functions computing the sizes for the individual bits, even those are mostly const. As it is, it's hard to tell whether everything is covered.
e.g. vsize (Value c v) = foldr accum (coinSize c) v where coinSize _ = uint.
| deriving (Show, Eq, ToCBOR, FromCBOR, Ord, NoUnexpectedThunks, NFData) | ||
|
|
||
| -- | The Value representing MultiAssets | ||
| data Value crypto = Value Coin (Map (PolicyID crypto) (Map AssetID Quantity)) |
There was a problem hiding this comment.
Is this normalized so that all Ada must be in the Coin part? Is the size calculation defined on the abstract type where we just have the map, or on this optimized version?
(Also, I thought we were only going to use the optimized one in specific places?)
| utxoEntrySizeWithoutVal = inputSize + outputSizeWithoutVal | ||
|
|
||
| -- This scaling function is right for UTxO, not EUTxO | ||
| scaleVl :: (Val v) => v -> Coin -> Coin |
There was a problem hiding this comment.
This is about the deposit? It's quite hard to work out whether this is doing the right thing since it's fairly obscure what it's supposed to do.
This commit only adds the class and an instance for `Coin`. It differs from #1815 in the following ways: - Class methods are now intended for qualified import (as per Michael's comment in #1803) - Added some additional documentation. Some functions still need significantly more documentation. - Use the `Semigroup`, `Monoid` and `Group` superclasses. This removes the need for various functions defined in this class. - Removed the instance for `Integer` and define the instance for `Coin` directly.
This commit only adds the class and an instance for `Coin`. It differs from #1815 in the following ways: - Class methods are now intended for qualified import (as per Michael's comment in #1803) - Added some additional documentation. Some functions still need significantly more documentation. - Use the `Semigroup`, `Monoid` and `Group` superclasses. This removes the need for various functions defined in this class. - Removed the instance for `Integer` and define the instance for `Coin` directly.
|
Closing this, as it's been superseded at least twice. We can still reference the code review suggestions made. |
Temporary Multiasset parametrization PR -- to see if CI tests give same output as locally